Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a custom matplotlib backend for positron #2765

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Apr 15, 2024

Intent

Address #2748.

Approach

This implements a custom matplotlib backend for Positron in the matplotlib_backend module. I believe this is the intended interface to connect matplotlib to a new frontend, and it allows us much tighter integration between matplotlib events and the Positron frontend.

I refactored the plots service (plots.py) to be independent of the plotting backend framework (in this case matplotlib). In theory, the plots service should easily be able to connect to other Python plotting frameworks.

I also added the show event to the plots comm to focus an existing plot without requesting a re-render.

See the linked issue description and tests for more details.

QA Notes

See the linked issue and the contributed tests for QA test cases. Note that notebook plotting behavior should be unaffected by this PR.

@petetronic
Copy link
Collaborator

petetronic commented Apr 15, 2024

On quick initial testing, this looks great, and answers a long standing question about plot incremental updates. Once you resolve the new conflicts, let's quickly chat - I think the need for show() by default is fine in the REPL... but want to chat about shared state between plt usages.

@seeM seeM force-pushed the custom-matplotlib-backend branch from 0322f0b to 6973964 Compare April 15, 2024 17:58
@seeM
Copy link
Contributor Author

seeM commented Apr 15, 2024

I've fixed the merge conflicts. Happy to chat!

Called by matplotlib when a figure is shown via `plt.show()` or `figure.show()`.
"""
if self._plot is None:
self._plot = self._plots_service.create_plot(self.canvas._render, self._handle_close)
Copy link
Collaborator

@petetronic petetronic Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this path, i.e. the first time, does self._plot.show() not get called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the explicit show isn't needed in that case because when the frontend receives the comm open message, it responds with a render request. I'll add a comment.

…ernel/matplotlib_backend.py

Signed-off-by: Wasim Lorgat <[email protected]>
@petetronic
Copy link
Collaborator

petetronic commented Apr 16, 2024

This is really great, one thing I'd like to brainstorm on is whether there are any enhancements in a future PR we can make to communicate the "current" context of matplotlib, i.e. which figure, or potentially more (axes etc). is current. When working on separate figures, some of the API seems to require you to change the "current" figure to mutate it, and keeping track of what is current may be useful feedback in the UI?

@seeM seeM merged commit f0f72c9 into main Apr 16, 2024
24 checks passed
@seeM seeM deleted the custom-matplotlib-backend branch April 16, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants